[VMware to KVM] Cleanup leftover migrated volumes in case of migration failures#13151
Conversation
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13151 +/- ##
=========================================
Coverage 18.94% 18.94%
- Complexity 18363 18366 +3
=========================================
Files 6192 6195 +3
Lines 556361 556451 +90
Branches 67908 67916 +8
=========================================
+ Hits 105397 105421 +24
- Misses 439393 439455 +62
- Partials 11571 11575 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17829 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16076)
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a best-effort cleanup path for temporary converted volumes when VMware→KVM import fails, and refactors KVM conversion wrappers to share common helper logic.
Changes:
- Trigger cleanup of temporary converted disks on import failures/timeouts.
- Introduce
CleanupConvertedInstanceDisksCommandand a corresponding KVM resource wrapper to delete leftover conversion artifacts. - Refactor
LibvirtImportConvertedInstanceCommandWrapperto reuse a new sharedLibvirtBaseConvertCommandWrapper.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java | Sends a new cleanup command to remove temporary converted disks when import fails. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapper.java | Refactors wrapper to inherit shared convert/import helpers and updates cleanup call signature. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCleanupConvertedInstanceDisksCommandWrapper.java | Adds KVM-side handler that locates and deletes temporary conversion disks (and XML when present). |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBaseConvertCommandWrapper.java | Introduces shared helper methods previously embedded in the import wrapper. |
| core/src/main/java/com/cloud/agent/api/CleanupConvertedInstanceDisksCommand.java | Adds agent command to request cleanup of converted disks by store + prefix. |
| core/src/main/java/com/cloud/agent/api/CleanupConvertedInstanceDisksAnswer.java | Adds a new Answer type for the cleanup command (currently empty). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
harikrishna-patnala
left a comment
There was a problem hiding this comment.
code LGTM. @nvazquez please check if we can add some unit tests here.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17841 |
|
@blueorangutan test |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18334 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16395)
|
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18356 |
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 18367 |
|
@blueorangutan package |
|
@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18370 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16428)
|
|
@nvazquez |
|
@nvazquez Could you rebase with 4.22 ? I am moving it to 4.22.2 milestone |
RosiKyu
left a comment
There was a problem hiding this comment.
LGTM
Executed the full test plan for FR307: VMWare-2-KVM part II - Cleanup after failure - 19/19 test cases Pass (100%).
Summary:
Testing covered cleanup of temporary artifacts (disks and XML descriptors) after failed VMware-to-KVM imports: temporary locations on the primary pool and NFS secondary storage, XML handling (present / absent / corrupt with fallback to prefix listing), forceConvertToPool positive and negative paths, mid-move multi-disk failure (customer repro), destination-pool orphan check, and correct cleanup scoping (concurrent imports and the convert-stage boundary remain untouched). Negative scenarios confirmed no crashes when there is nothing to clean or when cleanup itself fails, with the original error preserved. Failed conversions are correctly reflected in the UI and in listImportVmTasks, and the successful import regression passed with temporary artifacts fully cleaned.
Executed test cases:
- Concurrent imports cleanup scoped to failed prefix only
- Import fails after temporary disks already cleaned nothing to clean no crash (negative)
- Convert host equals import host VDDK same host cleanup works
- Import VM tasks UI reflects failed conversion
- listImportVmTasks shows Failed record on pool selection failure
- Convert-stage failure not cleaned scope boundary
- forceConvertToPool success no erroneous cleanup (positive)
- forceConvertToPool import failure cleans pool disks (negative)
- Successful import regression temporary fully cleaned (positive)
- Temporary location primary pool cleaned
- Temporary location NFS secondary cleaned (positive)
- Destination-pool orphan check after mid-move failure (negative)
- Mid-move multi-disk failure temporary fully cleaned customer repro
- Cleanup with corrupt XML falls back to prefix listing (CS-3416)
- Cleanup with XML absent removes disks skips XML
- Cleanup with XML present removes disks and XML (positive)
- Import failure agent unavailable cleans temporary disks
- Cleanup failure preserves original error and does not crash (negative)
- Import failure bad result cleans temporary disks (positive)
Description
This PR includes a cleanup mechanism for migrated volumes which are leftover in case the VMware to KVM migrations have failed unexpectedly.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?